misc: Move browser dropdown within AUT URL bar #31216
Conversation
cypress
|
||||||||||||||||||||||||||||||||||||||||
| Project |
cypress
|
| Branch Review |
browser-dropdown-ux-update
|
| Run status |
|
| Run duration | 18m 18s |
| Commit |
|
| Committer | Jennifer Shehane |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
10
|
Pending
|
1232
|
Skipped
|
0
|
Passing
|
32102
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
46.57%
|
|
|---|---|
|
Untested elements
|
182
|
|
Tested elements
|
163
|
Accessibility
92.57%
|
|
|---|---|
|
Failed rules
|
3 critical
8 serious
2 moderate
2 minor
|
|
Failed elements
|
885
|
…nt to work........
| @@ -151,7 +151,7 @@ describe('Cypress In Cypress E2E', { viewportWidth: 1500, defaultCommandTimeout: | |||
| }) | |||
|
|
|||
| it('shows a compilation error with a malformed spec', { viewportHeight: 596, viewportWidth: 1000 }, () => { | |||
| const expectedAutHeight = 456 // based on explicitly setting viewport in this test to 596 | |||
| const expectedAutHeight = 500 // based on explicitly setting viewport in this test to 596 | |||
There was a problem hiding this comment.
This has legitimately changed, previously the browser dropdown at this width was lessening the AUT height.
| @@ -128,7 +128,7 @@ export const longBrowsersList = [ | |||
| majorVersion: '69', | |||
There was a problem hiding this comment.
Not really necessary changes, but these mocks seem more realisitic
…eenshotting from breaking.
… to be above that also
| @@ -1,3 +1,4 @@ | |||
| <!-- Be careful with changing styles of the panels, it can impact our screenshot tests --> | |||
There was a problem hiding this comment.
lesson learned.......
…ss-io/cypress into browser-dropdown-ux-update
| @@ -61,14 +64,15 @@ import { Popover, PopoverButton, PopoverPanel } from '@headlessui/vue' | |||
| const props = withDefaults(defineProps<{ | |||
| variant?: 'panel' | |||
| align?: 'left' | 'right' | |||
|
|
|||
| minimal?: boolean | |||
There was a problem hiding this comment.
can we add a comment or two describing the new minimal option here?
There was a problem hiding this comment.
From what I can see one allows for a visual dropdown with a chevron vs the browser icon being the dropdown?
| const inputZIndex = computed(() => { | ||
| // input needs to be above the Studio prompt overlay | ||
| // but other times it needs to be below other resizable panels | ||
| return studioStore.needsUrl ? 51 : 5 |
There was a problem hiding this comment.
is the studio prompt overlay have a z-index of 50 or something near that? Is there a better way to determine the z-index value here over static values?
There was a problem hiding this comment.
Yah, the overlay has a 50 zindex. It is a good point, this code was so brittle with the zindex changes. I suffered a lot.
| const alphaSortedBrowser = (props.gql.browsers ?? []).slice().sort((a, b) => a.displayName > b.displayName ? 1 : -1) | ||
|
|
||
| // move the selected browser to the top to easily see selected browser version at the top when opening the dropdown | ||
| alphaSortedBrowser.some((browser, i) => browser.isSelected && alphaSortedBrowser.unshift(alphaSortedBrowser.splice(i, 1)[0])) |
There was a problem hiding this comment.
might make this easier to read/debug with something like this to see whats selected? Not as optimized but we are dealing with fairly small arrays. More of a nit if anything
| alphaSortedBrowser.some((browser, i) => browser.isSelected && alphaSortedBrowser.unshift(alphaSortedBrowser.splice(i, 1)[0])) | |
| const [selectedBrowser] = _.remove(alphaSortedBrowser, (browser) => browser.isSelected) | |
| alphaSortedBrowser.unshift(selectedBrowser) |
| @@ -123,7 +121,12 @@ const props = withDefaults(defineProps <{ | |||
| }) | |||
|
|
|||
| const browsers = computed(() => { | |||
| return (props.gql.browsers ?? []).slice().sort((a, b) => a.displayName > b.displayName ? 1 : -1) | |||
| const alphaSortedBrowser = (props.gql.browsers ?? []).slice().sort((a, b) => a.displayName > b.displayName ? 1 : -1) | |||
There was a problem hiding this comment.
not: maybe a sortBy makes this more readable?
| const alphaSortedBrowser = (props.gql.browsers ?? []).slice().sort((a, b) => a.displayName > b.displayName ? 1 : -1) | |
| const alphaSortedBrowser = _.sortBy(props.gql.browsers ?? [], 'displayName') | |
Additional details
Steps to test
yarn cypress:openin thepackages/appdirectoryHow has the user experience changed?
Before
After
Automation Missing error did not change much
PR Tasks
cypress-documentation?type definitions?